[For discussion] Add args from #392
Conversation
| @@ -54,15 +61,17 @@ | |||
| help='Amount of Scrapy Cloud units (-u number)') | |||
| @click.option('-t', '--tag', | |||
| help='Job tags (-t tag)', multiple=True) | |||
| def cli(spider, argument, set, environment, priority, units, tag): | |||
| @click.option('-f', '--args_from', | |||
There was a problem hiding this comment.
For consistency I'd suggest to use a hyphen, it'll be converted to args_from automatically
| @click.option('-f', '--args_from', | |
| @click.option('-f', '--args-from', |
| @@ -34,6 +34,13 @@ | |||
| Similarly, job-specific settings can be supplied through the -s option: | |||
|
|
|||
| shub schedule myspider -s SETTING=VALUE -s LOG_LEVEL=DEBUG | |||
|
|
|||
| Also, the spider can be run with all arguments are taken from another job with -f option: | |||
There was a problem hiding this comment.
Should we extend the logic to optionally inherit job-level settings as well (with a different option ofc)? Could be done separately, here I'm more worried about consistent API, say we reserve -f/--args-from for arguments, and -s/--settings-from for settings. Wdyt?
There was a problem hiding this comment.
I think this makes totally sense, thank you :). For settings we already have "-s", we can use "-g/--settings-from" for example.
Apart from the settings, there are also parameters that we can take from the "parent" job
--environment (-n, --env-from)
--priority
--units
--tag
Not sure about the priority, units, tag, but what do you think about the environment?
There was a problem hiding this comment.
Tbh the short flag -g here -g/--settings-from looks a bit misleading 🤔 Also inheriting different parameters from different jobs (say get tags from job A, get settings from job B etc) doesn't look useful in real life, you'd usually inherit parameters from a single job.
My suggestion here would be
- add a single key-value param specifiying a job which we should use as a base one, like
-i/--inherit-from - add boolean flags for different parameters, like
--inherit-args,--inherit-settings,--inherit-environment,--inherit-tags - inheriting priority/units doesn't seem to be useful, that's just one number, but I might be wrong there
- using
--inherit-fromwithout specifiying what exactly to inherit should fail with a warning
Your thoughts?
There was a problem hiding this comment.
Although the argument's names are a bit long (--inherit-environment) but clear and no problem to use this I hope. And the approach with --inherit argument looks good and quite agile.
About using --inherit-from without specifiying what exactly to inherit should fail with a warning - what do you think if instead of raising a warning it will inherit all parameters (args, settings, environment, tags, priority, units) at once? In most cases, we need exactly this behavior. Or just add a new argument like to --inherit-all (in addition to --inherit-args, etc).
There was a problem hiding this comment.
instead of raising a warning it will inherit all parameters
Looks good to me, let's go this way 👍🏻
| def add_args_from_job(client, base_args, args_from): | ||
| if not args_from: | ||
| return base_args | ||
| job_args = get_args_from_parent_job(client, args_from).copy() |
There was a problem hiding this comment.
Should we use copy.deepcopy here instead?
No description provided.